-
Notifications
You must be signed in to change notification settings - Fork 39
Add support for bitcoin core 29.0 #131
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
5438355 to
1dbf4df
Compare
|
CI fail can be fixed by creating the RPC help file, I haven't downloaded Core v29 yet but I'd expect that on this branch we could do: contrib/run-bitcoind.sh start v29
bt29 help > verify/rpc-api-v29.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Man this is looking pretty good! This crate is hard to work on so don't feel bad if you miss things. I'm having a hard time reviewing because the changeset is so big but I've left a few comments that will make it a fair bit smaller. Keep at it! Appreciate your effort.
I tried running the command after starting the bitcoind v29, but it keeps say the |
dc856f3 to
3c6c22f
Compare
|
Oh I have a shell alias for each version, for v28 I have bt28: aliased to bitcoin-cli -rpcconnect=localhost:28049 -rpcuser=*** -rpcpassword=***And fill in the stars for your setup. Remember the leading 280 comes from the config file of |
3c6c22f to
8135a58
Compare
Thank you... It's done! |
798a62e to
2308f10
Compare
20080f5 to
87a78dd
Compare
|
Can you get rid of all the formatting changes please. |
fb921bd to
5709498
Compare
Done! |
fa9350a to
1db66bf
Compare
|
It is surprising to see changes to |
Alright, so for v29, the |
|
Yes please. Anything that is different in one version to another should have a separate type, errors included. I'm not totally convince that we should re-use errors at all but we already do in submitpackage IIRC and its kinda nice. |
Alright, noted, I am going to do that right away... |
| /// The bits | ||
| pub bits: Option<CompactTarget>, // Only from v29 onwards | ||
| /// The difficulty target. | ||
| pub target: Option<Target>, // Only from v29 onwards |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate above.
|
Man it looks like you haven't rebased this for a while. You likely need to rebase on master then check a bunch of files using |
seems I had a mix up somewhere. Thank you for pointing these out. |
3e54aff to
521f814
Compare
|
The Most recent changes I made was to resolve pending issues, despite I rebase master earlier, seems somethings didn't apply the way they're supposed to, took care of that also. Thank you! |
b46666e to
94554b6
Compare
|
I pushed to your branch man, hope you don't mind. I'll wait till you take a look in case you want to ack my changes then if CI is green we can merge. |
|
ACK 40810ad. Thank you so much for pushing this forward... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Firstly an apology for jumping in at this late stage, it is looking good and I didn't see anything major. There are just a few things that should be changed before merging.
| //! We ignore option arguments unless they effect the shape of the returned JSON data. | ||
| pub mod blockchain; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| //! We ignore option arguments unless they effect the shape of the returned JSON data. | |
| pub mod blockchain; | |
| //! We ignore option arguments unless they effect the shape of the returned JSON data. | |
| pub mod blockchain; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooph, good eye!
integration_test/tests/blockchain.rs
Outdated
| #[cfg(not(feature = "v29"))] | ||
| #[test] | ||
| fn blockchain__get_blockchain_info__modelled() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| #[cfg(not(feature = "v29"))] | |
| #[test] | |
| fn blockchain__get_blockchain_info__modelled() { | |
| #[test] | |
| fn blockchain__get_blockchain_info__modelled() { | |
| #[cfg(not(feature = "v29"))] | |
| // Call to a pre v29 test function, i.e. what this test is now | |
| #[cfg(feature = "v29")] | |
| // Call to a v29 test |
The v29 rpc still exists so should be tested. It could be done as a follow up PR but then the table in types should say UNTESTED for getblockchaininfo in v29
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes this is better.
integration_test/tests/mining.rs
Outdated
| node1.mine_a_block(); | ||
| node2.mine_a_block(); | ||
| node3.mine_a_block(); | ||
| std::thread::sleep(std::time::Duration::from_millis(500)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| std::thread::sleep(std::time::Duration::from_millis(500)); |
What is this for? The test passes fine on my end without it.
NB. There is an intermittent error that causes a random test to fail, running the test again usually passes: failed to create node: it appears that bitcoind is not reachable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd guess its 'wait for nodes to start'? And I'd guess that its from an intermittent failure during dev. Ok to remove or keep but if any time was lost debugging in the past because of this I'd keep it. Should have a comment on it if we keep it. I believe I mentioned this before in review at some stage?
I just removed it.
verify/src/method/v29.rs
Outdated
| Method::new_numeric("uptime", "uptime"), | ||
| // mining | ||
| Method::new_modelled("getblocktemplate", "GetBlockTemplate", "get_block_template"), | ||
| Method::new_no_model("getmininginfo", "GetMiningInfo", "get_mining_info"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Method::new_no_model("getmininginfo", "GetMiningInfo", "get_mining_info"), | |
| Method::new_modelled("getmininginfo", "GetMiningInfo", "get_mining_info"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tcharding how is this handled when no model is required for earlier versions? Verify currently complains for all versions.
Although for this case it makes sense to add a model for all versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh this is wrong, thanks man. Changed to new_modelled for all versions and implemented into_model for the v17 type.
verify/src/method/v29.rs
Outdated
| Method::new_numeric("getconnectioncount", "get_connection_count"), | ||
| Method::new_no_model("getnettotals", "GetNetTotals", "get_net_totals"), | ||
| Method::new_modelled("getnetworkinfo", "GetNetworkInfo", "get_network_info"), | ||
| Method::new_no_model("getnodeaddresses", "GetNodeAddresses", "get_node_addresses"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Method::new_no_model("getnodeaddresses", "GetNodeAddresses", "get_node_addresses"), | |
| Method::new_modelled("getnodeaddresses", "GetNodeAddresses", "get_node_addresses"), |
Recent change in master needs to be added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Niiiice - done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed up all the mod.rs in types too.
types/src/v29/blockchain/error.rs
Outdated
| PreviousBlockHash(ref e) => | ||
| write_err!(f, "conversion of the `previous_bock_hash` field failed"; e), | ||
| NextBlockHash(ref e) => | ||
| write_err!(f, "conversion of the `next_bock_hash` field failed"; e), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| write_err!(f, "conversion of the `next_bock_hash` field failed"; e), | |
| write_err!(f, "conversion of the `next_block_hash` field failed"; e), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good eye, fixed in v17 also.
types/src/v29/mining/mod.rs
Outdated
| /// Maximum allowable input to coinbase transaction, including the generation award and transaction fees (in satoshis). | ||
| #[serde(rename = "coinbasevalue")] | ||
| pub coinbase_value: i64, | ||
| /// A list of supported features,for example `proporsal` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// A list of supported features,for example `proporsal` | |
| /// A list of supported features, for example `proposal` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doe.
types/src/v29/mining/mod.rs
Outdated
| pub coinbase_value: i64, | ||
| /// A list of supported features,for example `proporsal` | ||
| pub capabilities: Vec<String>, | ||
| /// ID to include with a request to longpool on an update to this template |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// ID to include with a request to longpool on an update to this template | |
| /// ID to include with a request to longpoll on an update to this template |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
types/src/v29/mining/mod.rs
Outdated
| pub capabilities: Vec<String>, | ||
| /// ID to include with a request to longpool on an update to this template | ||
| #[serde(rename = "longpollid")] | ||
| pub long_pool_id: String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| pub long_pool_id: String, | |
| pub long_poll_id: String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
types/src/v29/network.rs
Outdated
| @@ -0,0 +1,78 @@ | |||
| // SPDX-License-Identifier: CC0-1.0 | |||
|
|
|||
| //! The JSON-RPC API for Bitcoin Core `v28` - network. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| //! The JSON-RPC API for Bitcoin Core `v28` - network. | |
| //! The JSON-RPC API for Bitcoin Core `v29` - network. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
b3fd549 to
40810ad
Compare
|
Bother, we both force pushed. I went through you changes and I think I got all the ones that were correct. Can you not force push again please man. I'm going to get this through CI then merge it and we can follow up if needed. |
002dec9 to
1214d36
Compare
Add support for the latest version of Core. Along the way fix various bugs, typos, and mistakes repo wide. Co-authored-by: Tobin C. Harding <[email protected]>
|
I've made the PR into a single commit, you are the author still @GideonBature and I added myself using a co-developed-by tag. |
|
Phew - that was hard. |
This PR adds support for the newly released bitcoin core version - v29.0
Updated RPCs
getmininginfowhich now returns nBits and the current target in the target field. It also returns a next object which specifies the height, nBits, difficulty, and target for the next block. (test added)getblockandgetblockheaderwhich now return the current target in the target field (test added)getblockchaininfoandgetchainstateswhich now return nBits and the current target in the target field. (test added)getblocktemplatewhose RPC curtime (BIP22) and mintime (BIP23) fields now account for the timewarp fix proposed in BIP94 on all networks. (test added)New RPCs
getdescriptoractivityrpc method which can be used to find all spend/receive activity relevant to a given set of descriptors within a set of specified blocks. (test added)Closes #129